Omnibar position option #4885

0nko · 2024-08-09T16:09:44Z

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208356719296150/f

Description

This is a feature branch for the omnibar position project.

Testing instructions

Dummy toolbar

Start the omnibar at the top
Use the Fire button and notice the dummy toolbar is briefly shown at the top
Change the omnibar to the bottom
Use the Fire button and notice the dummy toolbar is briefly shown at the bottom

Autocomplete

Change the omnibar position to the bottom
Start typing in the address bar
Notice the suggestions are correctly displayed above the toolbar

UI changes

Before	After

Snackbar

Change the omnibar position to the bottom
Trigger a condition to show a snackbar (see the diff below)
Notice the snackbar is displayed above the omnibar

This patch will triggers a snackbar after tapping the fire button:

Index: app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
--- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt	(revision Staged)
+++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt	(date 1725012737445)
@@ -3299,6 +3299,7 @@
     }
 
     fun onFireMenuSelected() {
+        showErrorWithAction()
         val cta = currentCtaViewState().cta
         if (cta is OnboardingDaxDialogCta.DaxFireButtonCta) {
             onUserDismissedCta(cta)

UI changes

Before	After

Privacy pop-up dialog

Go to settings and change the omnibar position to BOTTOM
Go back to the tab
Trigger the privacy popup condition by refreshing the page
Notice the dialog correctly appears at the bottom

UI changes

Before	After

Onboarding messages

Clear the app storage and trigger onboarding
Change the omnibar position to the bottom
Go to “google.com"
Notice that during onboarding steps, whenever there is the “hand” icon, it points to the bottom

UI changes

Before	After

Feature flag tests

Bottom bar enabled

Use https://www.jsonblob.com/api/1287776240176848896 privacy config URL and the playRelease build, so that the feature flag is enabled

Go to Settings -> Appearance
Verify that there is the Address bar option to change the omnibar position

Bottom bar disabled

Use http://www.jsonblob.com/api/1287777925473361920 privacy config URL, so that the feature flag is enabled

Go to Settings -> Appearance
Verify that the Address bar option is not visible

0nko · 2024-08-09T16:09:58Z

Omnibar position option #4885 : 5 dependent PRs (#4886 , #4940 , #4941 and 2 others) 👈
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @0nko and the rest of your teammates on Graphite

malmstein

There are a couple of outstanding comments from previous PR.
The big issue is that when the omnibar is at the bottom we are over the web content, and not properly pushing it to the top. That makes it so any content at the bottom of the viewport is behind the omnibar.

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

malmstein · 2024-09-10T08:58:05Z

browser-api/src/main/java/com/duckduckgo/app/browser/omnibar/model/OmnibarPosition.kt

+ * limitations under the License.
+ */
+
+package com.duckduckgo.app.browser.omnibar.model


why is this in the browser-api module?

Based on what I observed, it seemed like the right place to put a common model that might be accessed by different modules in the future. If you feel it’s not the right place I’m open to suggestions.

If another module would need to access the omnibar position, they will do it via the SettingsDataStore. You have placed the model in the browser-api module, but the public accessor to that value is in the app module.

This module is specifically for things related to the browser. The omnibar and it’s position are separate from that. In other words, omnibar doesn’t need to know anything about the browser. I’d move it to the same app/omnibar package you have the rest of the logic.

0nko · 2024-09-12T10:24:59Z

The big issue is that when the omnibar is at the bottom we are over the web content, and not properly pushing it to the top. That makes it so any content at the bottom of the viewport is behind the omnibar.

@malmstein Thanks for the review. I will fix this in a separate PR. The rest of the comments should be addressed now.

I added steps for testing different scenarios in the PR description.

malmstein · 2024-09-18T21:11:18Z

Omnibar position option #4885 : 4 dependent PRs (#4886 , #4940 , #4941 and 1 other) 👈
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @0nko and the rest of your teammates on Graphite

…visible scrollable

…havior

…lbackPlugin

0nko · 2024-09-23T14:12:57Z

@malmstein I’ve updated the test instructions to include tests of the feature flag, as well.

malmstein

Looking good and works as expected. Thanks @0nko !

app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarFeatureFlagObserver.kt

0nko mentioned this pull request Aug 9, 2024

Omnibar position: Settings and store #4886

Merged

9 tasks

0nko force-pushed the feature/ondrej/omnibar-position branch 2 times, most recently from 99ecf5e to adaf626 Compare August 21, 2024 21:49

0nko mentioned this pull request Aug 22, 2024

Omnibar position: Bottom app bar layout #4930

Merged

6 tasks

0nko force-pushed the feature/ondrej/omnibar-position branch from adaf626 to e6f425a Compare August 23, 2024 10:21

0nko requested a review from malmstein August 23, 2024 10:37

This was referenced Aug 27, 2024

Omnibar position: Progress bar #4939

Merged

Omnibar position: Menu #4940

Merged

Omnibar position: Dummy toolbar #4941

Merged

Omnibar position: Onboarding messages #4942

Merged

0nko force-pushed the feature/ondrej/omnibar-position branch from efd808b to dcd1d5e Compare August 28, 2024 10:44

This was referenced Aug 28, 2024

Omnibar position: Autocomplete #4945

Merged

Omnibar position: Snackbar #4954

Merged

0nko force-pushed the feature/ondrej/omnibar-position branch from ed0ac91 to b906747 Compare August 29, 2024 15:12

0nko mentioned this pull request Aug 30, 2024

Omnibar position: Privacy popup dialog #4962

Merged

4 tasks

0nko force-pushed the feature/ondrej/omnibar-position branch 2 times, most recently from 1ab54e1 to 2b7eaca Compare September 2, 2024 12:37

0nko mentioned this pull request Sep 2, 2024

Omnibar position: Pixels #4974

Merged

11 tasks

0nko marked this pull request as ready for review September 9, 2024 16:00

0nko force-pushed the feature/ondrej/omnibar-position branch from 40ecd9e to b0dd837 Compare September 9, 2024 16:55

0nko mentioned this pull request Sep 9, 2024

Omnibar position: Translations #4993

Merged

malmstein requested changes Sep 10, 2024

View reviewed changes

0nko force-pushed the feature/ondrej/omnibar-position branch from 7bd0bd1 to cd4a537 Compare September 12, 2024 09:46

0nko requested a review from malmstein September 12, 2024 10:26

0nko mentioned this pull request Sep 12, 2024

Omnibar position: UI fixes #5003

Merged

0nko force-pushed the feature/ondrej/omnibar-position branch from 1672676 to 37114ff Compare September 17, 2024 14:33

0nko force-pushed the feature/ondrej/omnibar-position branch from 37114ff to 3864a65 Compare September 19, 2024 09:00

0nko added 12 commits September 23, 2024 14:43

Update the scrollability JS code to account for fixed position elements

8c30e28

Simplify the scrollability check

dceb715

Simplify the feature flag check and move it to a single place

f78bbac

Fix the scrollability check on certain sites by considering overflow …

1b6673c

…visible scrollable

Update the snackbar bottom padding

c13c88e

Fix ktlint issues

558898d

Fix the TopAppBarBehavior comment

c043780

Update the name of the scrollability check function to reflect the be…

4ca903b

…havior

Change the OmnibarFeatureFlagObserver plugin type to PrivacyConfigCal…

39857e7

…lbackPlugin

Fix the loading bar experiment

ada07f5

Enable the feature internally

b93b7eb

Fix ktlint issue

0175a60

0nko force-pushed the feature/ondrej/omnibar-position branch from d0678eb to 0175a60 Compare September 23, 2024 12:51

0nko mentioned this pull request Sep 23, 2024

Omnibar: Fix toolbar padding and transitions #5052

Merged

12 tasks

0nko requested review from malmstein and joshliebe September 23, 2024 13:02

0nko added 8 commits September 24, 2024 13:30

Animate the bottom margin along with the translation of the toolbar

824c248

Remove the JavaScript scrollability detection

28ba8ac

Fix the initial browser offset

93bd15f

Add bottom offset to all siblings of the omnibar if it's expanded

8f6cfbd

Remove the custom command to show the bottom bar

b798ff4

Fix ktlint issues

644c9e7

Only enable the omnibar feature if loading experiment disabled

fd113b6

Fix the unit tests

51b3b0e

malmstein approved these changes Sep 25, 2024

View reviewed changes

app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarFeatureFlagObserver.kt Show resolved Hide resolved

Reorder imports

ab8c7f2

0nko enabled auto-merge (squash) September 25, 2024 09:21

0nko merged commit 51036fd into develop Sep 25, 2024
6 checks passed

0nko deleted the feature/ondrej/omnibar-position branch September 25, 2024 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omnibar position option #4885

Omnibar position option #4885

0nko commented Aug 9, 2024 •

edited by malmstein

Loading

0nko commented Aug 9, 2024 •

edited

Loading

malmstein left a comment

malmstein Sep 10, 2024

0nko Sep 12, 2024

malmstein Sep 12, 2024

0nko commented Sep 12, 2024 •

edited

Loading

malmstein commented Sep 18, 2024

0nko commented Sep 23, 2024

malmstein left a comment

Omnibar position option #4885

Omnibar position option #4885

Conversation

0nko commented Aug 9, 2024 • edited by malmstein Loading

Description

Testing instructions

Dummy toolbar

Autocomplete

UI changes

Snackbar

UI changes

Privacy pop-up dialog

UI changes

Onboarding messages

UI changes

Feature flag tests

0nko commented Aug 9, 2024 • edited Loading

malmstein left a comment

Choose a reason for hiding this comment

malmstein Sep 10, 2024

Choose a reason for hiding this comment

0nko Sep 12, 2024

Choose a reason for hiding this comment

malmstein Sep 12, 2024

Choose a reason for hiding this comment

0nko commented Sep 12, 2024 • edited Loading

malmstein commented Sep 18, 2024

0nko commented Sep 23, 2024

malmstein left a comment

Choose a reason for hiding this comment

0nko commented Aug 9, 2024 •

edited by malmstein

Loading

0nko commented Aug 9, 2024 •

edited

Loading

0nko commented Sep 12, 2024 •

edited

Loading